Fix sooperlooper per-loop bindings #511
Conversation
Added ctrl:loop controllers for all per-loop bindable controllers (as
previously defined, there may benefit more).
But also simple 'ctrl' controllers to always handle the selected loop.
The mode of the binding to be made is still changed with
selected_loop_cc_binding (on the last page of controls).
It is indicated on per-loop bindable controls with (loop number) added
to their name.
This is updated whenever the loop selection is changed or when
selected_loop_cc_binding is set to True.
The earlier implementation was incorrect in multiple aspects:
1. It bound ctrl:loopnumbers only, and it only worked when the loop was
also already the selected one.
This check is now no longer made.
2. Checks for current state checked the selected loop, instead of the
targeted loop. The targeted loop (chan) is now set and checked.
2. Selecting a loop with another method than going to the loop
selection page and back to another controller page did not work
because the UI did not refresh the actual controllers.
Fixed this by calling
self.state_manager.send_cuia("refresh_screen", ["control"])
(I did not find a more direct way)
| return processor.controllers_dict | ||
|
|
||
| def adjust_controller_bindings(self): |
There was a problem hiding this comment.
I am not sure we should be adding more UI specific code into this backend engine class. We have been seperating UI from backend code. The ctrl screens present a challenge so have not yet been worked on.
There was a problem hiding this comment.
It is effectively the same as from the old code from line 849—854, with the added condition to set the non-loop-bound controller when selected_loop_cc_binding was True. Since this code has to run in two occasions (when toggling the option and when changing the selected loop), I turned it into a method.
There was a problem hiding this comment.
Okay - We should avoid this but it is already here in the old code so remains an issue to resolve when we split model from view.
| except IndexError: | ||
| return | ||
| processor.refresh_controllers() | ||
| self.state_manager.send_cuia("refresh_screen", ["control"]) |
There was a problem hiding this comment.
This is a UI specific call but, maybe this is a reasonable way to handle the challenge of backend changes impacting UI. I wonder whether the selected_loop_cc_binding code might be able to move to a UI class, triggered by this event?
There was a problem hiding this comment.
In my opinion, it is more of a UI change than a back-end change: something effecting what controllers are shown and, more importantly, what to bind to.
Without this, you have to manually refresh the screen.
There was a problem hiding this comment.
Thinking about this some more, I actually do not exactly follow what you mean. I do not see an event being fired, and I would not know how to add this logic to a new UI class.
The screen refresh is necessary for the new binding logic to work.
Something that probably went unnoticed before single_pedal mode was added to the per-loop binding (which is on the same screen as the loop selectors). For the other controls, you had to switch screens to select a loop and then get back to the screen with the desired controller. Which already does the desired refreshing, and thus the proper binding.
Adding the loop number to the visible name is just frills, and more of a result of me trying to get feedback while trying to get the binding logic right.
I'd really like to get this merged in in some way, so what's the way forward now?
There was a problem hiding this comment.
This seems like a reasonable way for the model (of which this engine is part) to send events to the view/controller (zyngui).
|
Yikes! I just merged but found issues with the control view gui widget which sends OSC commands to SL to select loops but the controls are not being updated by the correspoding osc message from SL. |
|
That's odd. i could swear I tested this with another program changing the selected loop via osc (my wip ctrldev) and the Zynthian GUI responding correctly. |
|
I have reverted vangelis. Please fix to work with control view widget. |
|
I just tested this, changing the loop from the widget (recorded via VNC). Controls do look updated:
|
|
When you select loop 6 the record control changes to ON but the widget does not show the loop as recording. |
|
I see, the controls themselves are being replaced, but the values of the controls are not (always) updated, probably when something happens to an unselected loop. Some complex stuff, I am looking into this. It indeed be nice if controllers just reacted to state changes. |
|
Update: I cannot reproduce or explain this very behaviour, yet. The opposite I can explain and reproduce: when changing a loop-bound control or state (via CC), selected-loop controls do not update their value, and neither do the widget buttons acquire the required state. EDIT: just fixed widget buttons getting the right state |
|
Update: I can kind of reproduce this when recording loop 6, removing loop 6, adding loop 6, and going back. |
This reverts commit afa8a79.
Besides ctrl:loop controllers for all per-loop bindable controllers (as previously defined, there may benefit more), also add simple, unnumbered, 'ctrl' controllers to always handle the selected loop.
The mode of the binding to be made is still changed with selected_loop_cc_binding (on the last page of controls).
It is indicated on per-loop bindable controls with (loop number) added to their name, e.g. 'record (3)'
This is updated whenever the loop selection is changed or when selected_loop_cc_binding is set to True.
The earlier implementation was incorrect in multiple aspects:
This check is now no longer made.
Fixed this by calling
self.state_manager.send_cuia("refresh_screen", ["control"])(I did not find a more direct way)
I noticed I inadvertently added IndexError, in order to not have a naked except (so vscode told me). Revert if this is an incorrect assumption.